Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change tool metadata file format to JSON #1553

Merged
merged 36 commits into from
Jan 3, 2025
Merged

Conversation

vicroms
Copy link
Member

@vicroms vicroms commented Dec 13, 2024

Continuation of #1490
Related: #1277

This PR contains several changes:

  • Moves vcpkgTools.xml data into a JSON file and removes the XML parsing code.
  • Adds the architecture field to the tool metadata.
  • Removes the requirement of force system binaries on arm64 Linux platforms.

@vicroms vicroms force-pushed the tools branch 2 times, most recently from 5fe30dc to 3747fae Compare December 13, 2024 09:46
@vicroms vicroms changed the title [WIP] Add ARM64 Linux tools [WIP] Change tool metadata file format to JSON Dec 13, 2024
@vicroms vicroms changed the title [WIP] Change tool metadata file format to JSON Change tool metadata file format to JSON Dec 16, 2024
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feature!

src/vcpkg/base/json.cpp Outdated Show resolved Hide resolved
src/vcpkg/vcpkgcmdarguments.cpp Show resolved Hide resolved
include/vcpkg/tools.test.h Show resolved Hide resolved
src/vcpkg-test/tools.cpp Outdated Show resolved Hide resolved
src/vcpkg/tools.cpp Outdated Show resolved Hide resolved
src/vcpkg/tools.cpp Outdated Show resolved Hide resolved
src/vcpkg/tools.cpp Outdated Show resolved Hide resolved
src/vcpkg/tools.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@autoantwort autoantwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a json schema for this file?

Comment on lines +61 to +67
"name": "cmake",
"os": "linux",
"version": "3.22.2",
"executable": "cmake-3.22.2-linux-x86_64/bin/cmake",
"url": "https://github.com/Kitware/CMake/releases/download/v3.22.2/cmake-3.22.2-linux-x86_64.tar.gz",
"sha512": "579e08b086f6903ef063697fca1dc2692f68a7341dd35998990b772b4221cdb5b1deecfa73bad9d46817ef09e58882b2adff9d64f959c01002c11448a878746b",
"archive": "cmake-3.22.2linux-x86_64.tar.gz"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this entry has no architecture entry this version would be used on arm64 devices. Maybe the arch field should be mandatory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's reasonable for there to be arch-less entries like scripts blobs or similar. A big reason to do this work is to enable arm64, but truly doing well there is going to imply minting arm64 binaries and similar; fully getting that on sounds like a separate feature though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i hadn't thought about scripts, so one must simply be careful when adding entries

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or user "arch" : "neutral" as an indicator for stuff which does not dependent on arch similar to how the vs channel manifest works)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think an explicit 'neutral' value is substantially clearer than just not having the field, and would be 'annoying' as we couldn't reuse the existing CPUArchitecture enum and friends.

@BillyONeal
Copy link
Member

BillyONeal commented Jan 3, 2025

To do:
Update Ninja version to one with ARM64 support.

I don't believe this is needed to land this work.

EDIT: Oh I see it's necessary due to

Removes the requirement of force system binaries on arm64 Linux platforms.

Test with full registry build.

I don't believe this is needed because all this stuff is already tested by the fetch tests.

@BillyONeal
Copy link
Member

Interestingly, finally breaking the <regex> dependency reduced size slightly, even with all this extra JSON parsing code:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants